fix: Log exception and exit #505
Conversation
There was a problem hiding this comment.
The code is of course fine.
The original use for the background task was to update a live plot. In this case, I don't think the task throwing an exception should cause the loop to exit.
Other background tasks, of course, might properly cause the loop to fail if the task fails (throws).
I think it's reasonable to always fail if the background task throws, since you can always wrap the "real" background task in a try/catch that just ignores all exceptions (presumably other than keyboard interrupt). We should make sure that the docs call out this behavior.
As an aside, I would vote (strongly!) against adding another optional argument that flagged whether or not the loop should exit on a background task exception... There are far too many options already.
|
The logging is a great idea. I agree with @alan-geller though that we shouldn't exit on background task failure, unless the things people are doing with background tasks have changed substantially - data collection should not be contingent on live plotting, especially since the user can manually quit the loop if they're not seeing what they want. Originally the code had the |
|
@alexcjohnson @alan-geller I am not sure I like the "once is never, twice is always." But let's not change it for now. |
According to this principle from Alej "once is never, twice is always." See pr comments.
giulioungaretti
force-pushed
the
fix/logBackgroundException
branch
from
24508f2 to
75f188c
Compare
Author: Giulio Ungaretti <[email protected]> fix: Log exception and exit (#505)
Author: Giulio Ungaretti <[email protected]> fix: Log exception and exit (#505)
Closes #504
Changes proposed in this pull request:
@alan-geller , and @QCoDeS/core the question here is if we want to exit (early return) or not.
My feeling is that we may as well not exit, technically the data could be still good even if the background task failed, but one never knows.
The clean way is to return, but I am curious about your 💎 feedback !